-
Notifications
You must be signed in to change notification settings - Fork 38k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log HTTP method in logging filters and revise log message format #23567
Conversation
After a quick discussion with @rstoyanchev, we have decided that we should simply always include the HTTP method in those logs messages. In other words, there's no need to make it optional. Can you please rework this PR accordingly? |
Team Decision: After some further internal brainstorming, we have decided on the following.
Format: Example: @napstr, please let us know if you are willing to rework this PR along those lines. Thanks |
Will do, I'll aim to get this done over the weekend :) Just to clarify, did you also decide to change the existing |
The Spring Team would also like to point out that the This detailed logging in the Consult the Logging section of the Spring Web MVC documentation for further details. |
Great!
Yes, we decided to change the delimiter from |
0995005
to
3fdca2f
Compare
3fdca2f
to
dd041ce
Compare
How does this look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look good.
I've requested simplifications to the tests. So please make those and perhaps add an additional test that's more inclusive -- for example, one that contains everything like the previously posted example: Before request [GET /foo?x=2, client=..., session=..., user=..., headers=..., payload=...]
.
Thanks
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Show resolved
Hide resolved
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Outdated
Show resolved
Hide resolved
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Outdated
Show resolved
Hide resolved
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Outdated
Show resolved
Hide resolved
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Outdated
Show resolved
Hide resolved
spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Outdated
Show resolved
Hide resolved
33fe100
to
4b1bcb4
Compare
@sbrannen thank you for the hints on how to improve usage of the available AssertJ api. I believe I applied all your suggestions now - let me know if anything is left. |
Glad to be of service.
I think that looks good now. I've accepted the changes and will merge the PR in the coming days. Enjoy your weekend! |
Ahoy,
I was surprised to neither find the http method in the log messages of the
AbstractRequestLoggingFilter
nor the option to turn them on.This looks like a fairly uncontroversial, fully optional and backwards compatible change to me so I did not open an issue up front. I'm happy to open one if more discussion is required.
Two things to note for the reviewer:
- [ ] I copy-pasted the method javadocs, and I'm not 100% sure if anything else needs to be done to getto work. I did some CTRL-Fing for the other values and nothing interesting came up.- [ ] I did not add a@since 5.2
annotation as I'm not sure which version this will hit. Just amend it in :)Cheers,
Napster
Btw, setting up the local build and running tests was a breeze, great docs!